Extend profiler toolbar item #135

chr-hertel · 2017-03-17T17:03:57Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Related tickets	-
Documentation	-
License	MIT

Depends on stopwatch plugin PR

What's in this PR?

Extended profiler toolbar item to display more details and duration

dbu

this looks very cool, thanks a lot! i tried to find where x-duration comes from, but don't find that in the php-http codebase.

dbu · 2017-03-17T18:11:38Z

Collector/StackPlugin.php


        $this->collector->addStack($stack);

        return $next($request)->then(function (ResponseInterface $response) use ($stack) {
            $stack->setResponse($this->formatter->formatResponse($response));
+            $stack->setResponseCode($response->getStatusCode());
+
+            if ($response->hasHeader('X-Duration')) {


where does the x-duration header come from?

ah, now i got it, this depends on php-http/stopwatch-plugin#3

Nyholm

Awesome. Thanks!

Is it ready for merge?

dbu · 2017-03-20T08:30:09Z

@Nyholm it depends on php-http/stopwatch-plugin#3 and we currently are trying to figure out how to track requests and responses in the plugin chain...

fbourigault · 2017-03-21T08:16:21Z

What do you thing about removing the stopwatch plugin from the profiled stacks and directly use the stopwatch component in the StackPlugin?

    public function handleRequest(RequestInterface $request, callable $next, callable $first)
    {
        $stack = new Stack(
            $this->client,
            $request->getMethod(),
            $request->getRequestTarget(),
            $this->formatter->formatRequest($request)
        );

        $this->collector->addStack($stack);
        $eventName = sprintf('%s %s', $request->getMethod(), $request->getUri()->__toString());
        $event = $this->stopwatch->start($eventName, 'php_http.request');
        return $next($request)->then(function (ResponseInterface $response) use ($stack, $event) {
            $stack->setResponse($this->formatter->formatResponse($response));
            $stack->setResponseCode($response->getStatusCode());
            
            $event->stop();
            $stack->setDuration($event->getDuration());

            return $response;
        }, function (Exception $exception) use ($stack) {
            $stack->setResponse($this->formatter->formatException($exception));
            $stack->setFailed(true);

            $event->stop();
            $stack->setDuration($event->getDuration());

            throw $exception;
        });
    }

Such implementation does not require changes neither in symfony nor in the stopwatch plugin :)

chr-hertel · 2017-03-21T20:08:26Z

@fbourigault yes, would be an easy solution and more convincing than extending symfony's stopwatch. thanks :)
but IMO it's still a valid option to manipulate the response in stopwatch-plugin. we're just adding plugin related meta information to response.
@dbu @Nyholm let me know what your favorite choice is and i will adjust those PRs.

dbu · 2017-03-22T07:21:40Z

i think using the stopwatch directly instead of the plugin sounds better to me. much less complicated and achieves the same functionality in a clean way. at that point we would not need the stopwatch plugin for the bundle at all anymore, right?

fbourigault · 2017-03-22T07:39:27Z

The dependency will still be required. See https://github.com/php-http/HttplugBundle/blob/master/DependencyInjection/HttplugExtension.php#L185-L187.

dbu · 2017-03-22T08:57:46Z

oh good. then we don't need to decide if it would be a BC break to change the dependency...

chr-hertel · 2017-03-22T11:14:00Z

alright, great! :) i'll take care of this the next days

fbourigault

Here is what I did on my laptop: master...fbourigault:profile-client
This is a work in progress and I didn't tested how it operates with discovery.

fbourigault · 2017-03-22T12:58:39Z

Collector/StackPlugin.php

+        $stack = new Stack(
+            $this->client,
+            $request->getMethod(),
+            $request->getRequestTarget(),


Isn't better to track the request target closer to the client? Plugins may change the target (BaseUrlPlugin). I have on my laptop something equivalent to improve the profiler display.

fbourigault · 2017-03-22T13:00:02Z

Collector/StackPlugin.php


        $this->collector->addStack($stack);

        return $next($request)->then(function (ResponseInterface $response) use ($stack) {
            $stack->setResponse($this->formatter->formatResponse($response));
+            $stack->setResponseCode($response->getStatusCode());


The same happens here. Tracking close to the client avoid the ErrorPlugin to transform 4xx and 5xx errors to exceptions.

Nyholm · 2017-04-19T11:21:09Z

I've added this to the 1.5 milestone. Can you see why the tests are failing?

fbourigault · 2017-04-19T11:35:56Z

Could you also rebase your branch to get thinks working with ProfileClient?

chr-hertel · 2017-04-19T12:43:51Z

rebase is done and tests are not failing anymore. (honestly no clue what happened there.)

any ideas how to deal with the stopwatch-plugin? i think we need to disabled or even remove it because it interferes with the current implementation. but we could also stop using debug.stopwatch service in ProfileClient and instantiate a separate stopwatch.

fbourigault · 2017-04-23T21:32:23Z

About the stopwatch, I would remove the plugin and mesure time in the StackPlugin.

EDIT: to be more accurate, measuring in the ProfileClient will give us the time it took in the low level client (i.e. excluding plugin time execution). Measuring in the StackPlugin will give us the time it took with the plugins. Maybe it worth having both to display more data to the user!

fbourigault

Nice work! Along with the few comments in the code, could you add a note into the changelog? Moreover I noticed something that looks weird to me in the Performance tab:

I can't explain why this happens yet.

fbourigault · 2017-04-24T19:44:09Z

Collector/ProfileClient.php

-            $this->collectExceptionInformations($exception, $stack);
+                return $response;
+            }, function (\Exception $exception) use ($event, $stack) {
+                $this->collectExceptionInformations($exception, $event, $stack);


Should we call $event->stop() here?

fbourigault · 2017-04-24T19:47:55Z

Collector/ProfileClient.php

+     */
+    private function getStopwatchEventName(RequestInterface $request)
+    {
+        return sprintf('%s %s', $request->getMethod(), $request->getUri()->__toString());


The -->toString() is useless as %s will cast the URI as string.

chr-hertel · 2017-04-28T10:37:50Z

@fbourigault Thanks for your review! i wasn't able to reproduce your issues exactly but i had quite similar effects caused by duplicate stopwatch event names, so i improved ProfileClient::getStopwatchEventName.
can you please test, if that fixes your problem as well?

dbu · 2017-04-28T15:18:11Z

Collector/Collector.php

+    /**
+     * @return int
+     */
+    public function getDurationSum()


could we call this getTotalDuration?

dbu

great job!

fbourigault · 2017-04-30T21:41:55Z

Thank you @chr-hertel.

Nyholm self-requested a review March 17, 2017 17:03

dbu reviewed Mar 17, 2017

View reviewed changes

Nyholm approved these changes Mar 20, 2017

View reviewed changes

dbu changed the title ~~Extend profiler toolbar item~~ WIP: Extend profiler toolbar item Mar 20, 2017

chr-hertel mentioned this pull request Mar 22, 2017

Added duration header php-http/stopwatch-plugin#3

Closed

fbourigault reviewed Mar 22, 2017

View reviewed changes

This was referenced Apr 1, 2017

add profile client #136

Merged

Update design on WebProfiler #93

Closed

Nyholm added this to the Version 1.5.0 milestone Apr 19, 2017

chr-hertel changed the title ~~WIP: Extend profiler toolbar item~~ Extend profiler toolbar item Apr 24, 2017

fbourigault suggested changes Apr 24, 2017

View reviewed changes

chr-hertel added 2 commits April 28, 2017 12:06

extend profiler toolbar item

6d14164

removed stopwatch

201b11e

review feedback

f45d808

dbu reviewed Apr 28, 2017

View reviewed changes

Collector/Collector.php Outdated

/**

* @return int

*/

public function getDurationSum()

Copy link

Collaborator

dbu Apr 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we call this getTotalDuration?

rename durationSum to totalDuration

52da00b

dbu approved these changes Apr 30, 2017

View reviewed changes

fbourigault approved these changes Apr 30, 2017

View reviewed changes

fbourigault merged commit d8a85ac into php-http:master Apr 30, 2017

chr-hertel deleted the extend_profiler branch April 30, 2017 23:09

This was referenced May 10, 2017

Updated change log #160

Merged

remove stopwatch plugin for discovered clients. #161

Merged

Extend profiler toolbar item #135

Extend profiler toolbar item #135

Uh oh!

Conversation

chr-hertel commented Mar 17, 2017 • edited by dbu Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What's in this PR?

Uh oh!

dbu left a comment

Choose a reason for hiding this comment

Uh oh!

dbu Mar 17, 2017

Choose a reason for hiding this comment

Uh oh!

dbu Mar 17, 2017

Choose a reason for hiding this comment

Uh oh!

Nyholm left a comment

Choose a reason for hiding this comment

Uh oh!

dbu commented Mar 20, 2017

Uh oh!

fbourigault commented Mar 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chr-hertel commented Mar 21, 2017

Uh oh!

dbu commented Mar 22, 2017

Uh oh!

fbourigault commented Mar 22, 2017

Uh oh!

dbu commented Mar 22, 2017

Uh oh!

chr-hertel commented Mar 22, 2017

Uh oh!

fbourigault left a comment

Choose a reason for hiding this comment

Uh oh!

fbourigault Mar 22, 2017

Choose a reason for hiding this comment

Uh oh!

fbourigault Mar 22, 2017

Choose a reason for hiding this comment

Uh oh!

Nyholm commented Apr 19, 2017

Uh oh!

fbourigault commented Apr 19, 2017

Uh oh!

chr-hertel commented Apr 19, 2017

Uh oh!

fbourigault commented Apr 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fbourigault left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fbourigault Apr 24, 2017

Choose a reason for hiding this comment

Uh oh!

fbourigault Apr 24, 2017

Choose a reason for hiding this comment

Uh oh!

chr-hertel commented Apr 28, 2017

Uh oh!

dbu Apr 28, 2017

Choose a reason for hiding this comment

Uh oh!

dbu left a comment

Choose a reason for hiding this comment

Uh oh!

fbourigault commented Apr 30, 2017

Uh oh!

Uh oh!

chr-hertel commented Mar 17, 2017 •

edited by dbu

Loading

fbourigault commented Mar 21, 2017 •

edited

Loading

fbourigault commented Apr 23, 2017 •

edited

Loading

fbourigault left a comment •

edited

Loading